Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: fix empty issuer/subject/infoAccess parsing #14473

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jul 25, 2017

Also issuerCertificate but that did not fit on the status line.

Fixes: #11771

Split off from #14447.

CI: https://ci.nodejs.org/job/node-test-pull-request/9342/

Also issuerCertificate but that did not fit on the status line.

Fixes: nodejs#11771
@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Jul 25, 2017
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

if (c.subject) c.subject = tls.parseCertString(c.subject);
if (c.infoAccess) {
if (c.subject != null) c.subject = tls.parseCertString(c.subject);
if (c.infoAccess != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you put a quick comment that != null  matches both null and undefined? I think that might not be newbie-friendly (also because in every guide about JS it is written to use ===).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a comment if you insist but we use the x != null idiom all over the code base. It's not unique to this PR and we don't call it out explicitly anywhere else.

@bnoordhuis bnoordhuis closed this Jul 27, 2017
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jul 27, 2017
Also issuerCertificate but that did not fit on the status line.

Fixes: nodejs#11771
PR-URL: nodejs#14473
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 27, 2017
Also issuerCertificate but that did not fit on the status line.

Fixes: #11771
PR-URL: #14473
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@refack
Copy link
Contributor

refack commented Jul 30, 2017

Landed in 06a684a

@MylesBorins
Copy link
Contributor

should this be backported to v6.x?

@bnoordhuis
Copy link
Member Author

Yes.

MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
Also issuerCertificate but that did not fit on the status line.

Fixes: #11771
PR-URL: #14473
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants